fix: remove go routines for RecheckTx #1553
Conversation
cmwaters
requested review from
staheri14 and
ninabarbakadze
and removed request for
a team
| @@ -713,34 +708,23 @@ func (txmp *TxMempool) recheckTransactions() { | |||
|
|
|||
| // Issue CheckTx calls for each remaining transaction, and when all the | |||
| // rechecks are complete signal watchers that transactions may be available. | |||
| go func() { | |||
| g, start := taskgroup.New(nil).Limit(2 * runtime.NumCPU()) | |||
There was a problem hiding this comment.
do you think it would be possible to avoid this issue without blocking by simply not using a taskgroup?
There was a problem hiding this comment.
To be watertight it should be blocking over the period that state is updating else as this is still a go routine, transactions may be submitted before updating to the latest nonce with the checktxstate
There was a problem hiding this comment.
isn't the mempool locked while the goroutine is still running?
There was a problem hiding this comment.
It could happen in practice that some of these CheckTx transactions happen while the mempool is locked but it's not guaranteed
| return nil | ||
| }) | ||
| for _, wtx := range wtxs { | ||
| wtx := wtx |
There was a problem hiding this comment.
This line is not necessary any more b/c this repo uses Go 1.23 and Go 1.22 included:
Previously, the variables declared by a “for” loop were created once and updated by each iteration. In Go 1.22, each iteration of the loop creates new variables, to avoid accidental sharing bugs. The transition support tooling described in the proposal continues to work in the same way it did in Go 1.21.
Ref: https://tip.golang.org/doc/go1.22
| wtx := wtx |
oops disregard, I see this was just how it was previously implemented
Upgrade to https://github.com/celestiaorg/celestia-core/releases/tag/v1.44.1-tm-v0.34.35 to pick up this fix celestiaorg/celestia-core#1553 (cherry picked from commit 9f3b196)
Closes: #1552